Code Review — PR #21: Upgrade dependencies, update tools with new API features

Overall this is a well-structured modernisation PR. The centralised api.ts, the move to registerTool with I/O schemas, and the native-fetch migration all move in the right direction. A few items need attention before merging.

Overview

🔴 Critical

1. Entire test suite deleted with no replacement

src/utils.test.ts is deleted alongside vitest, and the test script is removed from package.json. The codebase now has no automated test coverage at all. The JSONStringify utility being deleted is fine, but the testing infrastructure going with it is a significant regression. Even a minimal smoke test for apiQuery's error paths and the buildQueryString helper would be worthwhile. Please add tests (or at minimum re-add vitest) before merging.

🟠 Bugs / Correctness

2. Schema-validation ApiError carries the wrong status code

In src/api.ts:

const parsed = opts.schema.safeParse(raw)
if (!parsed.success) {
  throw new ApiError(`Response validation failed: ${parsed.error.message}`, res.status, raw)
}

This path is only reached when res.ok is true, so res.status is a 2xx code. Every downstream catch block then checks error.status === 4xx and misses this case, falling through to the generic throw new Error(...). Callers will see a confusing message like "Response validation failed: ... (200)". Consider using a dedicated sentinel (e.g. 0 or 422) or a separate error class so callers can distinguish validation failures from HTTP errors.

3. list_shared_preconditions validates against a sub-schema, not the full output schema

const sharedPreconditions = await apiQuery(
  `/api/public/v0/project/${projectCode}/shared-precondition`,
  {
    schema: listSharedPreconditionsOutputSchema.shape.sharedPreconditions,  // ← array schema
    ...
  }
)
const result = { sharedPreconditions }

The raw API returns an array, but the output schema wraps it in { sharedPreconditions: [...] }. Passing .shape.sharedPreconditions works around this, but it means structuredContent validation against outputSchema (done by the SDK) will fail because the returned result shape differs from the declared outputSchema. Either align the API expectation with the schema, or adjust the output schema to match what the API actually returns.

4. list_folders error handler silently swallows 401/403

All other tools explicitly re-throw user-friendly messages for 401/403. list_folders only handles 404:

} catch (error: unknown) {
  if (error instanceof ApiError) {
    if (error.status === 404) {
      throw new Error(`Project with code '${projectCode}' not found.`)
    }
    throw new Error(`Failed to fetch test case folders: ${error.message}`)  // 401/403 land here
  }

An auth failure gives "Failed to fetch test case folders: Unauthorized" rather than the standard "Invalid or missing API key" message. Consistent with other tools, this should handle 401/403 explicitly.

🟡 Design / Maintainability

5. Duplicated error-handling pattern across every tool

Every tool file implements the same if (error instanceof ApiError) { if (error.status === 401) throw ... } block. With 10+ tools this is hard to keep consistent (as issue #4 above shows). Consider a shared helper:

function rethrowApiError(error: unknown, context: string, projectCode?: string): never {
  if (error instanceof ApiError) {
    if (error.status === 401) throw new Error('Invalid or missing API key')
    if (error.status === 403) throw new Error('Insufficient permissions or suspended tenant')
    if (error.status === 404 && projectCode)
      throw new Error(`Project with code '${projectCode}' not found.`)
    throw new Error(`${context}: ${error.message}`)
  }
  throw error
}

6. testStepSchemaShape getter creates a new Zod schema object on every access

export const testStepSchemaShape = {
  // ...
  get subSteps() {
    return z.array(testStepSchema).optional().describe('...')
  },
}

The getter is needed to break the circular reference, but it constructs a new schema instance every time testStepSchemaShape.subSteps is read (e.g. when building the full tool I/O schema at startup). Cache it after first creation, or construct the lazy reference directly inside testStepSchema using z.lazy():

const testStepSchema: z.ZodType<TestStep> = z.object({
  ...
  subSteps: z.lazy(() => z.array(testStepSchema)).optional(),
})

7. Content-Type: application/json sent on GET requests

const headers: Record<string, string> = {
  Authorization: `ApiKey ${QASPHERE_API_KEY}`,
  'Content-Type': 'application/json',  // unnecessary for GET
}

Harmless but semantically incorrect. Consider only setting it when opts.body !== undefined.

🟢 Positive observations

• Centralising all HTTP logic in src/api.ts is a clear win — much easier to add retries, timeouts, or logging in one place.
• Zod I/O schemas on every tool are a great improvement; MCP clients can now introspect tool contracts.
• Replacing the ad-hoc field presence checks with proper Zod parsing is strictly better than what was there before.
• buildQueryString correctly handles undefined values and arrays — clean implementation.
• Removing types.ts and deriving types from Zod schemas eliminates the drift risk between the two.
• Breaking changes are clearly enumerated in the PR description — good migration documentation.

Summary

The architecture is solid. The three items that should be addressed before merging:

1. Add back a test suite (at minimum re-add vitest and cover apiQuery error paths).
2. Fix the ApiError status code for schema-validation failures.
3. Fix the list_shared_preconditions schema mismatch (sub-schema vs output schema).

The duplicated error handler and the Content-Type/getter issues are worth cleaning up but are not blockers.